Turn eager normalization errors to delayed errors #82039

estebank · 2021-02-12T19:09:47Z

When normalizing unions with fields coming from associated types that
don't satisfy trait bounds, avoid ICEing by using delay_span_bug()
instead of bug!().

Fix #81199.

rust-highfive · 2021-02-12T19:09:50Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

lcnr · 2021-02-13T11:34:32Z

compiler/rustc_passes/src/stability.rs

                        }
                    }
+                    _ => {


is this actually reachable? Do we have a test case where hir::ItemKind::Union ends up as ty::Error?

Also curious about this. Was this getting hit? If not, can you revert?

IIRC, I added this because it is reached after removing the other ICE.

lcnr · 2021-02-13T11:39:42Z

compiler/rustc_traits/src/normalize_erasing_regions.rs

@@ -40,7 +41,13 @@ fn normalize_generic_arg_after_erasing_regions<'tcx>(
                debug_assert!(!erased.needs_infer(), "{:?}", erased);
                erased
            }
-            Err(NoSolution) => bug!("could not fully normalize `{:?}`", value),


this bug is quite valuable and helped us catch some bugs with const generics, so I would really like to keep it that way.

We pretty much only emit a bug here if the type is not well formed, and I think normalize_erasing_regions should only be used on types that are well formed (even if there is another error).

I think having delay_span_bug here should be fine. This mimics bug with -Ztreat-err-as-bug right?

We get there with const evaluatable bounds when emitting errors, but these errors should be silent when encountered during selection, so just always causing an ICE here is preferable for me.

We shouldn't hit this, even after having a compilation error. Having a delay_span_bug here hides other bugs in the compiler.

We get there with const evaluatable bounds when emitting errors, but these errors should be silent when encountered during selection, so just always causing an ICE here is preferable for me.

I'm not sure I follow. Could you rephrase your position? It reads to me like

We get there with const evaluatable bounds when emitting errors

these errors should be silent when encountered during selection

always causing an ICE here is preferable

means

when we emit errors with const evaluatable bounds we get here

trait selection should silence this errors

we should always ICE when we hit these

which to me is saying two different things?

Would it be reasonable to only delay_span_bug on stable/beta and make it bug! in nightly?

I think what @lcnr means is that the error we get here would be silenced by trait selection, but we don't want that and should ICE.

I would be extremely wary of having different behavior on stable vs nightly.

We get there with const evaluatable bounds when emitting errors,

it is an implementation error for const evaluatable bounds to reach this bug! and we still don't know all the places where this happens. Often when reaching this bug! compilation would fail anyways so a delay_span_bug would not trigger in these cases. That would mean that we would only get ICE here once they get triggered by const evaluatable bounds during selection (which then succeeds using a different candidate). This happens a lot less often though, so it would take us longer to find all of these cases. Does this make more sense this way?

lcnr · 2021-02-13T11:58:01Z

compiler/rustc_passes/src/stability.rs

+                                .ty_adt_def()
+                                .map_or(false, |adt_def| adt_def.is_manually_drop())
+                                && !field_ty
+                                    .is_copy_modulo_regions(self.tcx.at(DUMMY_SP), param_env)


I think this is the actual problem here. imo is_copy_modulo_regions doesn't make sense outside of codegen/mir building.

This is currently a stability hole as the following compiles:

#[derive(Clone)] struct Bar<'a>(&'a ()); impl Copy for Bar<'static> {} union Foo<'a> { field: u32, other_field: Bar<'a>, }

For some reason we also explicitly require types to be copy when initializing a union, so the following does error:

fn test<'a>(q: &'a ()) { let bar = Bar(&q); let foo: Foo<'a> = Foo { other_field: bar, }; }

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=45ea9990bc47e6066abeec085a372b85

Does using the following also fix this ICE?

let trait_ref = TraitRef::new(copy_def_id, tcx.intern_substs(&[field_ty.into()])); let is_copy = tcx.infer_ctxt().enter(|infcx| { infcx.predicate_must_hold_considering_regions(Obligation::new( ObligationCause::dummy(), param_env, trait_ref.without_const().to_predicate(tcx), )) });

I don't think this is the root issue, but I agree that this seems like a potential soundness hole. I would maybe just file an issue for followup.

I don't think this is the root issue

My understanding is that the root issue is calling codegen_fulfill_obligations outside of codegen. I am a bit confused about why I said that is_copy_modulo_regions is the issue here when there's a call to normailize_erasing_regions right below it - as you've said in #82039 (comment). I personally prefer to "correctly" deal with regions in needs_drop and similar methods, even if that is a more complex change.

Yes, I agree. We shouldn't be erasing regions in needs_drop and that is the more correct fix here.

bors · 2021-02-17T01:35:29Z

☔ The latest upstream changes (presumably #81611) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC-zz · 2021-04-26T10:30:48Z

@estebank this is waiting for you to reply to the review and resolve the conflicts

lcnr · 2021-05-24T18:19:01Z

I am not able to review any PRs in the near future. I don't think there is anything a reviewer has to do for this PR right now

r? @matthewjasper

JohnCSimon · 2021-06-27T20:54:36Z

ping again from triage:
@estebank this is waiting for you to reply to the review and resolve the conflicts

camelid · 2021-07-25T00:00:15Z

FYI, @jyn514 found a very small minimization in #87327 (comment) that may be useful as a test case:

union U {
    a: <Self as Iterator>::Item,
}

jackh726 · 2021-08-25T02:39:10Z

r? @jackh726

jackh726

A couple comments but this looks good enough to me that I'm good with merging

jackh726 · 2021-08-25T02:40:17Z

compiler/rustc_passes/src/stability.rs

                        }
                    }
+                    _ => {


Also curious about this. Was this getting hit? If not, can you revert?

jackh726 · 2021-08-25T02:41:19Z

compiler/rustc_passes/src/stability.rs

+                                .ty_adt_def()
+                                .map_or(false, |adt_def| adt_def.is_manually_drop())
+                                && !field_ty
+                                    .is_copy_modulo_regions(self.tcx.at(DUMMY_SP), param_env)


I don't think this is the root issue, but I agree that this seems like a potential soundness hole. I would maybe just file an issue for followup.

jackh726 · 2021-08-25T02:49:00Z

compiler/rustc_passes/src/stability.rs

+                                && !field_ty
+                                    .is_copy_modulo_regions(self.tcx.at(DUMMY_SP), param_env)
+                            {
+                                if field_ty.needs_drop(self.tcx, param_env) {


So, under the presumption that we should only be calling normalize_erasing_regions on well-formed types, is the problem actually in needs_drop...?
Here:

rust/compiler/rustc_ty_utils/src/needs_drop.rs

Line 143 in faa0a10

let subst_ty = tcx.normalize_erasing_regions(

jackh726 · 2021-08-25T02:49:43Z

compiler/rustc_traits/src/normalize_erasing_regions.rs

@@ -40,7 +41,13 @@ fn normalize_generic_arg_after_erasing_regions<'tcx>(
                debug_assert!(!erased.needs_infer(), "{:?}", erased);
                erased
            }
-            Err(NoSolution) => bug!("could not fully normalize `{:?}`", value),


I think having delay_span_bug here should be fine. This mimics bug with -Ztreat-err-as-bug right?

When normalizing `union`s with fields coming from associated types that don't satisfy trait bounds, avoid ICEing by using `delay_span_bug()` instead of `bug!()`. Fix rust-lang#81199.

JohnCSimon · 2021-09-12T07:31:15Z

triage:

jackh726 left a comment
A couple comments but this looks good enough to me that I'm good with merging

setting to waiting on review

jackh726 · 2021-09-17T02:14:48Z

Okay after the discussion in this thread, particularly with @lcnr, I'm now of mixed minds about this PR. Let me explain my thought process.

First, as a bit of context to my thoughts, it's important to note that there are currently two real "ways" to normalize things: either through rustc_trait_selection::traits::project or rustc_trait_selection::traits::query::normalize (which under the hood uses the same machinery). These two entrypoints to normalize have different goals: the former is designed around the case of "try to normalize, if we can, we'll try later when we have more information; if there are required obligations for normalization, we can solve them later too" and the latter is "we must be able to normalize everything; if we can't it's an error. Furthermore, we must be able to prove all obligations." Unfortunately, normalize_erasing_regions bugs on Error. In the medium-to-long term, we should work to solidify the expected invariants here and eventually just unify these two entrypoints (but that's definitely not in scope for this PR at all).

The current expectation here is that everything can get normalized in normalize_erasing_regions; rather than making an exception for this one case, maybe we should take an alternative approach. Can we call query::normalize directly from needs_drop?

Another alternative is that the stability logic here isn't in the right place (should it be delayed until after we type check correctly?)

jackh726 · 2021-12-04T18:50:04Z

The issue was fixed with #91462. Closing this.

rust-highfive assigned lcnr Feb 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2021

lcnr reviewed Feb 13, 2021

View reviewed changes

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 26, 2021

bstrie added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 12, 2021

rust-highfive assigned matthewjasper and unassigned lcnr May 24, 2021

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 11, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2021

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2021

rust-highfive unassigned matthewjasper Aug 25, 2021

rust-highfive assigned jackh726 Aug 25, 2021

jackh726 approved these changes Aug 25, 2021

View reviewed changes

Turn eager normalization errors to delayed errors

717f97c

When normalizing `union`s with fields coming from associated types that don't satisfy trait bounds, avoid ICEing by using `delay_span_bug()` instead of `bug!()`. Fix rust-lang#81199.

estebank force-pushed the issue-81199 branch from c9b508b to 717f97c Compare August 25, 2021 15:21

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2021

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2021

jackh726 mentioned this pull request Nov 26, 2021

Implement version of normalize_erasing_regions that allows for normalization failure #91255

Merged

jackh726 closed this Dec 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn eager normalization errors to delayed errors #82039

Turn eager normalization errors to delayed errors #82039

estebank commented Feb 12, 2021

rust-highfive commented Feb 12, 2021

lcnr Feb 13, 2021

jackh726 Aug 25, 2021

estebank Aug 26, 2021

lcnr Feb 13, 2021 •

edited

Loading

jackh726 Aug 25, 2021

lcnr Aug 25, 2021

estebank Aug 26, 2021

jackh726 Aug 26, 2021

lcnr Aug 26, 2021

lcnr Feb 13, 2021

jackh726 Aug 25, 2021

lcnr Aug 25, 2021

jackh726 Aug 26, 2021

bors commented Feb 17, 2021

Dylan-DPC-zz commented Apr 26, 2021

lcnr commented May 24, 2021

JohnCSimon commented Jun 27, 2021

camelid commented Jul 25, 2021

jackh726 commented Aug 25, 2021

jackh726 left a comment

jackh726 Aug 25, 2021

jackh726 Aug 25, 2021

jackh726 Aug 25, 2021

jackh726 Aug 25, 2021

JohnCSimon commented Sep 12, 2021

jackh726 commented Sep 17, 2021

jackh726 commented Dec 4, 2021

Turn eager normalization errors to delayed errors #82039

Turn eager normalization errors to delayed errors #82039

Conversation

estebank commented Feb 12, 2021

rust-highfive commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Feb 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 17, 2021

Dylan-DPC-zz commented Apr 26, 2021

lcnr commented May 24, 2021

JohnCSimon commented Jun 27, 2021

camelid commented Jul 25, 2021

jackh726 commented Aug 25, 2021

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCSimon commented Sep 12, 2021

jackh726 commented Sep 17, 2021

jackh726 commented Dec 4, 2021

lcnr Feb 13, 2021 •

edited

Loading